Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP7 Support #48

Closed
wants to merge 75 commits into from
Closed

PHP7 Support #48

wants to merge 75 commits into from

Conversation

Sean-Der
Copy link
Member

Hey @laruence !
I have finally gotten this to the point that I feel confident about the architecture.

There are still many test fails (pass rate is at around 30%) However, most of these test fails have to do with not properly handling references and other issues that can be fixed easily (I just don't understand the zend engine API well enough)

I am opening this branch so I can mark some questions for you, and start a light review.

Would it be possible to start running some CI against this using the current service?

I am removing all preprocessor stuff to handle different API versions, I am only supporting PHP 7.

Sean-Der added 30 commits May 3, 2015 06:12
…hp_session functions also expect smart_string instead of char* + int
… zend_string instead of char* + len. IS_BOOL is not a distinct type anymore IS_TRUE/IS_FALSE instead
…e, and zend_string instead of char+int for zend_lookup_class
…nd msgpack_unserialize_raw to have one level less of dereference
…sgpack_unpacker_fetch_object and msgpack_base_fetch_object
…nd_parse_parameters, which returns a zend_string instead of char+int
…was being fired when zend_symtable_update succeeded instead of failed
@Sean-Der
Copy link
Member Author

Sean-Der commented Jun 1, 2015

Hey @laruence, first off good news! We are at 85% with test fails, holy crap I never thought I would see the day.... https://gist.github.com/Sean-Der/cd94c0b6fb71a81e089b

However, I have ran into another issue with translating. In PHP 5 you could call zend_symtable_update and zend_hash_index_update on an object, allowing you to create invalid properties. In PHP 7 when you use those functions on the HASH_OF(object) it will create strange state (object becomes immutable, you will get a segfault in other places)
Here is the sample code in PHP 5 showing these behaviors.

In PHP 7 converting these functions to the appropriate zend_object functions also breaks references. zend_std_write_property
Here is the sample code in PHP 5 showing these behaviors.


I am going to be taking a break from working on this extension till I know the answer to the issues I have talked about so far. The template related functions have memory leaks (that are caught by the PHP memory manager)

thanks!

@@ -492,61 +364,41 @@ static ZEND_METHOD(msgpack_unpacker, execute)
{
char *str = NULL, *data;
long str_len = 0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

str_len should be size_t

@Sean-Der
Copy link
Member Author

Sean-Der commented Jun 3, 2015

@zxcvdavid Thank you so much for looking this PR over! Going through and fixing your comments now.

I am not able to compile with -C89 , I am getting errors in Zend headers? What compiler/flags are recommended?

Would you mind looking over my comments/questions above about the zend API.

thanks!

@zxcvdavid
Copy link

@Sean-Der about C89 issues, you can see : php/php-src@76a290d

Variable definitions need at the top.

thanks.

@laruence
Copy link
Member

laruence commented Jun 9, 2015

Hey, just FYI, I am going to work on this in this week , I am going to create a new PHP7 branch.. thanks

@Sean-Der
Copy link
Member Author

Sean-Der commented Jun 9, 2015

@laruence that is awesome to hear!

Is there anything I can do to make things easier on you?


Also currently php-src has some headers with C++ style comments (can't compile anything with -ansi) I am going to open a PR soon to fix that. There was some discussion in the PHP 7 extension channel about it.

@laruence
Copy link
Member

laruence commented Jun 9, 2015

@Sean-Der nothing now, you have done a great work :), thanks

@laruence
Copy link
Member

just for the record, I spent whole day on this. there is a problem that is, we should pop values on stack. thus in case of err, we could release these garbages on the stack to fix the memleaks, which should reduce the peak memory usage as well.

however, it is not easy for the current implementation.. need more time..

my current work could be found at : https://github.com/msgpack/msgpack-php/tree/php7

@Sean-Der
Copy link
Member Author

@laruence Just looking at all the things you fixed up, I made SO many mistakes thank you for cleaning up after me :/ doesn't look like I saved you that much time in the end urgh, better luck next time hopefully!

sorry I couldn't figure out a better way to keep the stack an array of zval* , I am excited to see what the proper thing to do was!

thank you so much for putting so much time into this.

@laruence
Copy link
Member

nah, you did a great progress. just maybe because I am more familiar with PHP7 internal. :)
anyway, are you still willing to participated in maintaining this ext?

@laruence
Copy link
Member

@Sean-Der I fixed the memleaks, now I am going to close this PR.. thanks for all the thing you did, cheers!

@laruence laruence closed this Jun 11, 2015
@Sean-Der
Copy link
Member Author

@laruence working with you has been the best experience I have had with Open Source in a very long time. I can't thank you enough for how supporting/nice you have been. You have been awesome, if I run into you a conference I owe you a soda/beer etc...

I would love to continue helping with the extension! (Is that the most helpful thing I could be doing for PHP right now?)

If so I will work on getting bin/str support in 5+7 and take a look at all existing bugs and triage them. Otherwise, just tell me where I can be most useful! I am currently porting pecl-yaml and in contact with the maintainer, and things are going MUCH faster.

@Sean-Der Sean-Der deleted the php7-support branch June 11, 2015 04:34
@laruence
Copy link
Member

@Sean-Der great, I am going to invite you into this project :) thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants